feat(desktop): re-land virtualized timeline to fix macOS beachball#1250
feat(desktop): re-land virtualized timeline to fix macOS beachball#1250wpfleger96 wants to merge 6 commits into
Conversation
be2d9e5 to
cbfc4e6
Compare
f4927af to
77a5801
Compare
Port the virtualized timeline subsystem onto main's day-group render structure, re-threading the read-marker work through the virtualized rows. main builds every row synchronously on first mount, so cold channel-switch cost climbs with channel depth; virtualization renders only the visible window, making cost independent of depth. Ports timelineItems/scrollConvergence (+ lib tests), useLoadOlderOnScroll, useConvergentScrollToMessage, and the virtualizer-index restore in useAnchoredScroll. main's unread-counter fix is preserved, confined to the unread-count increment block. The two perf-hoist props the reference branch passed into MessageRow are dropped: virtualization already bounds rendered rows to the visible window, so the hoist optimizes a cost the mechanism eliminates, and MessageRow stays untouched. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The timeline-virtualization acceptance gate is a same-harness delta (header-arm longest-longtask <= B + 15ms), but the instrument that produced the baseline was ad-hoc and never committed, so it evaporated between sessions. Commit it so the gate's own instrument survives. Measures main-thread longtasks during the first (cold) switch into the 600-message deep-history channel under 4x CPU throttle, windowed to the 300-row ceiling. Reports median-of-5 longest-longtask, run-to-run spread, and total-longtask-time-in-window. Instrument, not a gate: the only assertion confirms the switch exercised the mount under throttle. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ized timeline A sticky header inside the scroll container drifts ~49px at scrollTop 0: once older history prepends and the scroll restores, the header pins to its clamped offset, but at the top it had been sitting at its larger natural flow offset. The fix portals the header into a non-scrolling overlay container outside the scroll element (mirroring the unread-pill overlay), so it pins to a fixed offset regardless of scroll position and cannot move as content prepends above the anchor. The per-scroll re-render that resolves the topmost visible day stays localized to VirtualizedList rather than forcing MessageTimeline to re-render on every scroll. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The timeline-virtualization port regressed scroll-anchoring: it dropped
the non-virtualized list's implicit "full content height exists at pin
time" invariant. Rows mount estimate-sized and measure to real height on
scroll, so the raw `scrollTo(scrollHeight)` mount pin landed short and
`scrollHeight` grew as off-screen rows measured.
Drive the mount bottom-pin through `virtualizer.scrollToIndex(lastIndex,
{align:"end"})` so TanStack lands the true last row through its own
measurement. Arm the existing settle-guard on smooth `scrollToBottom`
too — an animated jump is not atomic, so a mid-animation `onScroll`
latched a stale mid-history anchor that the ResizeObserver then restored.
Teach the prepend-restore loop to re-aim at the bottom when the user
abandons to bottom mid-restore, and the all-gone fallback to keep a
windowed-out anchor (vs. only pinning on genuine deletion).
The teleport spec's `scrollHeight <= baseline+100` setup proxy assumed
the non-virtualized contract (scrollHeight changes only on DOM adds); a
virtualizer grows `getTotalSize()` from lazy measurement alone. Replace
it with the direct in-flight signal the suite already keys on. Seed loops
that omitted `createdAt` collided on one whole-second stamp and sorted by
random UUID, so the asserted last/target row landed at a random index
often outside the virtualized window — make them monotonic to match the
file's own channel-intro seed precedent. No product-property assertion
text changed.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…lization The prior helper asserted symmetric balance (gapAbove == gapBelow within 1px), valid when intro, divider, and first row shared one flow layout. The virtualization re-land moved the divider and first row into the translateY track while the intro stays a flex sibling, so the two gaps are no longer comparable quantities. The fix had collapsed the oracle to bare non-overlap (>= 0 on both gaps), which gutted the layout-regression guard the test exists to provide. Source measurement showed the intro -> divider gap is the layout-controlled 8px and rock-stable, while the divider -> message gap is ~0 by construction (virtualizer rows are back-to-back) plus MessageRow avatar/font render jitter, genuinely variable run-to-run. So band the stable gap (8 +/- 2) as the real guard and keep the variable one as a non-overlap reading-order check. Renamed to match what it now verifies. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The keyed scroll container remounts per channel, but the virtualizer and the anchored-scroll ResizeObserver are owned by the parent and persisted across switches, so on switch-back they kept pointing at the previous channel's detached nodes. The mount-pin then fired scrollToIndex against a stale virtualizer (scrollElement on a detached node), landing at the top instead of the bottom, and the late-measurement bottom-chase never ran — so the top-anchored channel intro painted when it should be windowed out. Match the JS objects' lifetimes to the scroll node's: key TimelineMessageList on channelId so the virtualizer remounts fresh, register it in a layout effect so the parent's same-commit mount-pin reads the fresh instance, and add channelId to the observer deps so it re-observes the live content node. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
77a5801 to
bb941d4
Compare
| // virtualizer (e.g. the thread panel), there's nothing to converge — the DOM | ||
| // path is the whole story and a missing row simply isn't reachable. | ||
| const { scrollToMessage: convergeToMessage, cancel: cancelConvergence } = | ||
| useConvergentScrollToMessage(getVirtualizer, { |
There was a problem hiding this comment.
I think the convergence path needs an abandonment callback. useAnchoredScroll sets convergingTargetIdRef when the row is not mounted and the virtualizer takes over, and both the layout restore and ResizeObserver then cede while that ref is non-null. But this hook can also stop with onAbandoned when the id never appears in indexByMessageId (bad/stale link, deleted target, or frame cap), and this caller doesn't supply onAbandoned, so the route target is never cleared and convergingTargetIdRef stays set indefinitely. After that the normal anchor restore keeps bailing, which can leave bottom-stick / resize / append restoration disabled for the rest of that channel view.
Could we handle abandonment by clearing the route target (or otherwise telling useAnchoredScroll to clear its convergence ref) the same way convergence success calls onTargetReached?
| {group.label ? <DayDivider label={group.label} /> : null} | ||
| {group.rows.map((row) => ( | ||
| <TimelineRenderRowView | ||
| React.useEffect(() => { |
There was a problem hiding this comment.
This looks like it defeats the mount bottom-pin path. timelineItems starts as null in the parent and is only populated by TimelineMessageList from a passive useEffect (onItems?.(itemsResult)). But useAnchoredScroll performs its initial pin in a parent layout effect during the same first commit, and pinToBottomByIndexRef.current is derived from virtualizerOption during render. On the first mount for a channel, virtualizerOption is therefore still null (or worse, still the previous channel's item stream on a channel switch), so the initial pin falls back to the raw scrollHeight path instead of scrollToIndex(lastIndex, { align: "end" }).
That seems to undermine the “land at true bottom on mount/switch-back” fix this PR is trying to add. Could we make the flattened item stream available synchronously to the parent (or reset/report it in a layout-timed path before the parent mount pin runs) so lastIndex is for the current rendered list on the first pin?
Re-lands timeline virtualization on the desktop client, eliminating the macOS scroll beachball caused by rendering the entire message list into the DOM. Only the visible window is now mounted, and scroll anchoring is rebuilt to behave correctly under virtualization.
What changes
TimelineMessageListandVirtualizedListmount only the windowed rows instead of the full history.MessageTimelinelands at the true bottom on mount viascrollToIndex(lastIndex, { align: "end" })so the initial view is pinned correctly on the TanStack measurement path.useAnchoredScroll,useConvergentScrollToMessage,useLoadOlderOnScroll, andscrollConvergencepreserve the user's scroll position across history loads and deep-link/teleport navigation, with a settle guard so smooth-scroll animations can't latch a mid-animation anchor.ActiveDayHeaderprojects the current day divider as a drift-immune floating header over the virtualized list.cold-switch-longtask.perf.ts) measures channel-switch main-thread cost so the beachball budget is guarded going forward.scroll-history.spec.tsthat omittedcreatedAtcollided on a single whole-second stamp and sorted by random UUID — latent on the non-virtualized DOM, but decisive once only the windowed rows are mounted. Those loops now seed monotoniccreatedAt, matching the existing in-file precedent, with assertions unchanged. Thechannels.spec.tsintro/day-divider spacing oracle is re-expressed to band the layout-controlledgap-2(8px) spacing and assert non-overlap on the back-to-back row gap.Known follow-ups (non-blocking)
>=comparator in the anchor restore path is looser than its comment claims.Supersedes #1123 (abandoned hybrid attempt).